Skip to content

[scripts] Update generate-artifacts-executor script to account for optional output path #54609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 9 commits into from
Closed

[scripts] Update generate-artifacts-executor script to account for optional output path #54609

wants to merge 9 commits into from
+70 −35

Conversation

@kitten
Copy link
Contributor

@kitten kitten commented Nov 19, 2025 •
edited
Loading

Summary:

The argument for the output path to this CLI has always been optional, but the types didn't reflect this, which caused a few regressions in recent changes (e.g. #53503)

There's several code paths where the baseOutputPath can be altered or derived in different ways, which wasn't respected in logic before the main codegen output block or in generateReactCodegenPodspec.

When applied, this PR alters the baseOutputPath logic to use a default by computeBaseOutputPath extracted from computeOutputPath and have an added process.env.RCT_SCRIPT_OUTPUT_DIR override, which was previously only applied to readReactNativeConfig

This should restore:

  • react-native codegen being run without an --outputPath argument
  • codegen running with a package.json:codegenConfig.outputDir override

Resolves #54473

Changelog:

[GENERAL] [FIXED] - Ensure codegen CLI supports unspecified --outputPath argument and respects codegenConfig.outputDir configurations

Test Plan:

  • Manually run react-native codegen in rn-tester

@kitten
Update baseOutputPath input to generate-artifacts-executor to be …
37624e0 
…optional

The argument for the output path was always optional. This wasn't
reflected in types, however, so this was missed in two prior changes.
@meta-cla meta-cla bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 19, 2025
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Nov 19, 2025
@kitten kitten force-pushed the @kitten/fix/optional-base-output-path branch from b64d3f9 to 397b02c Compare November 19, 2025 17:30
@meta-codesync
Copy link

meta-codesync bot commented Nov 19, 2025

@cortinico has imported this pull request. If you are a Meta employee, you can view this in D87456501.

cipolleschi
cipolleschi requested changes Nov 21, 2025
View reviewed changes
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix. I left a few suggestions and a couple of questions.

kitten and others added 5 commits November 21, 2025 13:25
@kitten @cipolleschi
Apply suggestions from code review
1f28ede 
Co-authored-by: Riccardo Cipolleschi <[email protected]>
@kitten
speculative: Pass generated outputPath to generateReactCodegenPodspec
aca955b
@kitten
Remove redundant defaulting in generate-artifacts-executor/index.js
27fb60c
@kitten
Unify/restructure how the baseOutputPath is derived
69fee4c 
The main motivation here is to ensure that:
- `RCT_SCRIPT_OUTPUT_DIR` always takes precedence, as it should already
  be the correct output path
- `autolinking.json` is read correctly from altered `baseOutputPath`s
- `codegenConfig` overrides are consistently taken into account

This necessitates moving most logic into the `platforms` for-loop, as
the `baseOutputPath` can be changed per-platform in
`package.json:codegenConfig.outputDir[platform]`, but otherwise nothing
else changes.
@kitten
Revert making outputPath arguments optional
d5577cf
@kitten
Copy link
Contributor Author

kitten commented Nov 24, 2025

@cipolleschi: The main thing I was struggling with is that the baseOutputDir is structured into multiple separate places, but it's inconsistently defaulted in the for-loop later. The problem is that we only take the platform into account in the for-loop, but there we can alter the baseOutputPath (via codegenConfig), whereas we otherwise default to the argument (which, of course, as it turned out, is meant to be optional)

To circumvent this, I went for the other approach, I originally deemed to invasive, which is to unify the computeBaseOutputPath logic (extracted from computeOutputPath) and to pull all logic down into the for-loop

@kitten kitten requested a review from cipolleschi November 24, 2025 17:58
cipolleschi
cipolleschi approved these changes Nov 25, 2025
View reviewed changes
Copy link
Contributor

@cipolleschi cipolleschi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the code make sense to me. I'm a bit concerned about the caching mechanism, but we will know if it produces issues.

@cortinico
Copy link
Contributor

cortinico commented Nov 25, 2025

@cipolleschi can you re-improt this one?

@meta-codesync meta-codesync bot closed this in 7163033 Nov 26, 2025
@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Nov 26, 2025
@meta-codesync
Copy link

meta-codesync bot commented Nov 26, 2025

@cortinico merged this pull request in 7163033.

This was referenced Nov 26, 2025
Open
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cipolleschi cipolleschi cipolleschi approved these changes

Assignees

No one assigned

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Codegen CLI can not be run without the --outputPath parameter specified since 0.81.2.

4 participants

@kitten @cortinico @cipolleschi @facebook-github-bot